IB: check every patch is marked in one pass and one reduction by sbryngelson · Pull Request #1933 · MFlowCode/MFC

sbryngelson · 2026-10-01T02:15:49Z

Summary

Follow-up to #1914, addressing its open Copilot review finding.

s_check_every_patch_marked looped over every global patch, and for each one swept every local cell and ran an MPI reduction. That is num_gbl_ibs grid sweeps plus num_gbl_ibs collectives at setup, and num_gbl_ibs counts every particle-cloud particle (up to 54,000 with the configured limit). The check now:

makes one pass over the local cells, counting markers by patch id;
reduces the whole count array in one collective;
aborts if any patch's count is zero.

It also decodes each marker with s_decode_patch_periodicity before counting. The old check compared the raw marker with == gid, so the cells of a body wrapped across a periodic boundary did not count toward their patch, and a fully wrapped image could wrongly abort a valid run.

Counts are integer(kind=8), reduced with a new s_mpi_allreduce_integer_sum_vec in m_mpi_common (shaped like s_mpi_allreduce_min_vec, with the same non-MPI fallback). The abort and its message are unchanged, and the 16-line comment is cut to three.

The check runs once, from s_ibm_setup, not per time step. So the old cost was a setup-time cost, and the check catches a body that marks no cell at startup, not one that later leaves the domain.

Verification

pre_process, simulation and post_process build on master d87a5fea with this change on Frontier, in all three configurations: CPU (--no-gpu), OpenMP offload (--gpu mp) and OpenACC (--gpu acc).
./mfc.sh precheck passes (all 7 gates).
All 163 IBM and Example tests pass (CPU, run as CI runs them on Frontier). Every one of them runs this check at setup.
Negative case: a radius-0.002 disc at a cell corner on a 50×50 grid (dx = 0.02), which contains no cell centre, still aborts on 2 ranks with the same message. The same disc centred on a cell centre marks that cell and runs normally.

AI disclosure

Written with Claude Code (Anthropic).

Acknowledgement

I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

s_check_every_patch_marked swept every local cell and ran a reduction once per global patch, and num_gbl_ibs counts every particle-cloud particle. Count markers by decoded patch id in one pass and reduce the counts in one collective. Decoding also counts the cells of a body wrapped across a periodic boundary, which the raw == gid test missed. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 4 · 1 · 3

Open (8)

Counts are being accumulated and reduced in real(wp), then compared via exact equality to… · New
Counts are being accumulated and reduced in real(wp), then compared via exact equality to… · New
Counts are being accumulated and reduced in real(wp), then compared via exact equality to… · New
Counts are being accumulated and reduced in real(wp), then compared via exact equality to… · New
This introduces an unconditional MPI allreduce call where the previous code was guarded by `#ifdef… · New
The per-patch counts are modeled as 2D arrays with a hard-coded second dimension of 1, which… · New
The per-patch counts are modeled as 2D arrays with a hard-coded second dimension of 1, which… · New
The per-patch counts are modeled as 2D arrays with a hard-coded second dimension of 1, which… · New

What changed in this PR

Optimizes s_check_every_patch_marked to avoid per-patch grid sweeps and collectives by counting marked cells per patch in a single local pass and performing a single global reduction, while correctly handling periodic-encoded markers.

Changes:

Replace num_gbl_ibs repeated sweeps/reductions with one local sweep that bins counts by patch id.
Perform a single collective reduction over the full per-patch count array.
Decode marker periodicity before counting so wrapped images count toward their owning patch.

File	Description
`src/simulation/m_ibm.fpp`	Refactors “every patch marked” validation to single-pass counting + single reduction and adds periodicity-aware marker decoding.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

sbryngelson · 2026-10-01T03:19:55Z


-        integer(kind=8) :: cnt_loc, cnt_glb
-        integer         :: gid, i, j, k
+        real(wp), allocatable :: cnt_loc(:,:), cnt_glb(:,:)


Done in 97d7106: counts are now 1-D integer(kind=8), reduced with a new s_mpi_allreduce_integer_sum_vec, and tested with == 0_8. (The real-valued version could not misreport, since a sum of non-negative counts is zero only if all of them are, but integers are the right type.)

+        allocate (cnt_loc(num_gbl_ibs, 1), cnt_glb(num_gbl_ibs, 1))
+        cnt_loc = 0._wp


+                do i = 0, m
+                    if (ib_markers%sf(i, j, k) /= 0) then
+                        call s_decode_patch_periodicity(ib_markers%sf(i, j, k), gid)
+                        cnt_loc(gid, 1) = cnt_loc(gid, 1) + 1._wp


-                       & "An immersed boundary marked no cell anywhere: its centroid decides which rank "// "owns it, so a body placed elsewhere with model_translate is handed to a rank that "// "does not hold it. Set patch_ib%x/y/z_centroid to where the body actually is.")
        end do
+        call s_mpi_allreduce_vectors_sum(cnt_loc, cnt_glb, num_gbl_ibs, 1)
+        @:PROHIBIT(any(cnt_glb(:, 1) == 0._wp), &


sbryngelson · 2026-10-01T03:19:57Z

-            @:PROHIBIT(cnt_glb == 0_8, &
-                       & "An immersed boundary marked no cell anywhere: its centroid decides which rank "// "owns it, so a body placed elsewhere with model_translate is handed to a rank that "// "does not hold it. Set patch_ib%x/y/z_centroid to where the body actually is.")
        end do
+        call s_mpi_allreduce_vectors_sum(cnt_loc, cnt_glb, num_gbl_ibs, 1)


No change here. s_mpi_allreduce_integer_sum_vec, like the other m_mpi_common reductions, has an #else branch that copies the input when MFC_MPI is undefined, so non-MPI builds work. This check runs once, in s_ibm_setup, so one collective on a single rank costs nothing.

sbryngelson · 2026-10-01T03:19:58Z

-                        if (ib_markers%sf(i, j, k) == gid) cnt_loc = cnt_loc + 1_8
-                    end do
+        if (num_gbl_ibs == 0) return
+        allocate (cnt_loc(num_gbl_ibs, 1), cnt_glb(num_gbl_ibs, 1))


Done in 97d7106: the counts are 1-D now. The trailing dimension came from the 2-D real-valued helper, which the new integer reduction replaces.

+                do i = 0, m
+                    if (ib_markers%sf(i, j, k) /= 0) then
+                        call s_decode_patch_periodicity(ib_markers%sf(i, j, k), gid)
+                        cnt_loc(gid, 1) = cnt_loc(gid, 1) + 1._wp


-                       & "An immersed boundary marked no cell anywhere: its centroid decides which rank "// "owns it, so a body placed elsewhere with model_translate is handed to a rank that "// "does not hold it. Set patch_ib%x/y/z_centroid to where the body actually is.")
        end do
+        call s_mpi_allreduce_vectors_sum(cnt_loc, cnt_glb, num_gbl_ibs, 1)
+        @:PROHIBIT(any(cnt_glb(:, 1) == 0._wp), &


Use 1-D integer(kind=8) counts and a new s_mpi_allreduce_integer_sum_vec (shaped like s_mpi_allreduce_min_vec, with the same non-MPI fallback) instead of real(wp) counts in a (num_gbl_ibs, 1) array, as review suggested. Co-Authored-By: Claude <noreply@anthropic.com>

codecov · 2026-10-01T09:59:15Z

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.82%. Comparing base (ed7a238) to head (97d7106).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/common/m_mpi_common.fpp	66.66%	1 Missing ⚠️
src/simulation/m_ibm.fpp	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1933      +/-   ##
==========================================
- Coverage   62.82%   62.82%   -0.01%     
==========================================
  Files          86       86              
  Lines       22394    22398       +4     
  Branches     3305     3305              
==========================================
+ Hits        14070    14072       +2     
- Misses       6071     6072       +1     
- Partials     2253     2254       +1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-10-05T00:38:16Z

Lines of Code

File	Lines	Diff
`src/common/m_mpi_common.fpp`	1494	+10
`src/simulation/m_ibm.fpp`	1442	+3

Directory	Lines	Diff
common	10426	+10
simulation	27979	+3
total	46933	+13

Copilot AI balanced review requested due to automatic review settings October 1, 2026 02:15

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Copilot started reviewing on behalf of sbryngelson October 1, 2026 05:50 View session

Merge branch 'master' into fix/ib-marked-check-single-pass

35401cb

Navigation Menu

IB: check every patch is marked in one pass and one reduction - #1933

IB: check every patch is marked in one pass and one reduction#1933
sbryngelson wants to merge 3 commits into
MFlowCode:masterfrom
sbryngelson:fix/ib-marked-check-single-pass

sbryngelson commented Oct 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

sbryngelson Oct 1, 2026

Uh oh!

sbryngelson Oct 1, 2026

Uh oh!

sbryngelson Oct 1, 2026

Uh oh!

codecov Bot commented Oct 1, 2026

Uh oh!

github-actions Bot commented Oct 5, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

		allocate (cnt_loc(num_gbl_ibs, 1), cnt_glb(num_gbl_ibs, 1))
		cnt_loc = 0._wp

Conversation

sbryngelson commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

AI disclosure

Acknowledgement

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

sbryngelson Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

sbryngelson Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

sbryngelson Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 1, 2026

Codecov Report

Uh oh!

github-actions Bot commented Oct 5, 2026

Lines of Code

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

sbryngelson commented Oct 1, 2026 •

edited

Loading